Skip to content

Remove narrow-to-wide formatter specializations - #41143

Draft
benhillis wants to merge 8 commits into
microsoft:masterfrom
benhillis:user/benhill/remove-narrow-wide-formatters
Draft

Remove narrow-to-wide formatter specializations#41143
benhillis wants to merge 8 commits into
microsoft:masterfrom
benhillis:user/benhill/remove-narrow-wide-formatters

Conversation

@benhillis

Copy link
Copy Markdown
Member

Summary

Follow-up to #41071. Remove the custom narrow-to-wide std::formatter specializations entirely and make cross-encoding explicit at the appropriate boundary.

Generated localization helpers now adapt char*, character arrays, std::basic_string<char>, and std::basic_string_view<char> arguments through wsl::shared::string::MultiByteToWide. Adapted values are materialized as local variables before make_wformat_args so their storage remains valid through vformat.

Direct wide std::format, Reporter, and user-error paths convert narrow values explicitly.

Validation

  • Clean full x64 Debug build
  • WSLCTests::InvalidNames
  • WSLCVolumeMountUnitTests::VolumeMount_IsValidNamedVolumeName_InvalidNames
  • UnitTests::WslInfo

Copilot AI review requested due to automatic review settings July 22, 2026 21:27
@benhillis
benhillis requested review from a team as code owners July 22, 2026 21:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes the custom std::formatter narrow→wide specializations and makes UTF-8→UTF-16 conversions explicit at formatting boundaries (localization helpers, user-error paths, reporters, and tests), avoiding collisions with libc++’s deleted cross-encoding formatters.

Changes:

  • Removed charwchar_t std::formatter specializations and updated the std::source_location wide formatter to explicitly convert function_name()/file_name().
  • Updated generated localization helpers to materialize converted arguments before make_wformat_args / vformat.
  • Updated Windows WSLC code paths and tests to explicitly call wsl::shared::string::MultiByteToWide where wide formatting/user errors/reporting consume narrow values.

Reviewed changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tools/generateLocalizationHeader.ps1 Generates WIN32-only format-argument adapters that materialize narrow→wide conversions before vformat.
src/shared/inc/stringshared.h Removes narrow→wide std::formatter specializations; updates std::source_location wide formatter to explicitly convert.
src/windows/wslcsession/WSLCSession.cpp Converts Docker error strings to wide before THROW_HR_WITH_USER_ERROR* / warnings.
src/windows/wslcsession/WSLCContainer.cpp Converts Docker error strings to wide before THROW_HR_WITH_USER_ERROR*.
src/windows/wslcsession/DockerHTTPClient.h Converts Docker exception message to wide before user-error macro.
src/windows/wslc/services/ImageProgressCallback.cpp Explicit narrow→wide conversions for reporter output and formatted status lines.
src/windows/wslc/services/BuildImageCallback.cpp Explicit narrow→wide conversions for reporter output.
src/windows/wslc/services/ContainerModel.cpp Makes user-error messages wide by converting narrow strings explicitly.
src/windows/wslc/services/ContainerService.cpp Removes implicit formatting conversion by passing narrow args to localized helper directly.
src/windows/wslc/services/SessionService.cpp Removes implicit formatting conversion by passing narrow args to localized helper directly.
src/windows/wslc/services/WinCredStorage.cpp Removes implicit formatting conversion by passing narrow args to localized helper directly.
src/windows/wslc/services/FileCredStorage.cpp Removes implicit formatting conversion by passing narrow args to localized helper directly.
src/windows/wslc/tasks/ImageTasks.cpp Converts image IDs to wide before wide reporter output.
src/windows/wslc/tasks/NetworkTasks.cpp Removes implicit formatting conversion by passing narrow args to localized helper directly.
src/windows/wslc/tasks/RegistryTasks.cpp Removes implicit formatting conversion by passing narrow args to localized helper directly.
src/windows/wslc/tasks/VolumeTasks.cpp Removes implicit formatting conversion by passing narrow args to localized helper directly.
src/windows/wslc/commands/VersionCommand.cpp Converts package version to wide for wide reporter output.
src/windows/common/WSLCUserSettings.cpp Removes implicit formatting conversion by passing e.what() directly to localized helper.
test/windows/Common.h Updates unordered-vector mismatch formatting to explicitly convert std::string to wide for diagnostics.
test/windows/Common.cpp Updates pattern-mismatch diagnostics to explicitly convert narrow strings to wide.
test/windows/WSLCTests.cpp Converts narrow values to wide when formatting test diagnostics / expected strings.
test/windows/UnitTests.cpp Converts narrow values to wide where wide formatting is used.
test/windows/SimpleTests.cpp Converts narrow values to wide where wide formatting is used.
test/windows/DrvFsTests.cpp Converts narrow paths to wide where wide formatting is used.
test/windows/wslc/e2e/WSLCE2EWarningTests.cpp Removes extra conversion at call site now handled by localized helper / explicit conversions.
test/windows/wslc/e2e/WSLCE2EImageDeleteTests.cpp Converts container/image IDs to wide when formatting expected stderr.
test/windows/wslc/e2e/WSLCE2EHelpers.cpp Converts narrow error messages/IDs to wide for logging/expected output.
test/windows/wslc/e2e/WSLCE2EGlobalTests.cpp Converts WSL_PACKAGE_VERSION to wide for formatted output.
test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp Converts prompt to wide when formatting interactive command line.
test/windows/wslc/e2e/WSLCE2EContainerExecTests.cpp Converts prompt/IDs to wide when formatting command lines and expected stderr.
test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp Converts registry address/prompt to wide when formatting command lines.
test/windows/wslc/e2e/WSLCE2EContainerAttachTests.cpp Converts prompt to wide when formatting command line.

Comment thread tools/generateLocalizationHeader.ps1
Comment thread src/windows/wslc/services/ContainerModel.cpp
Comment thread src/windows/wslc/services/ContainerModel.cpp Outdated
@benhillis
benhillis marked this pull request as draft July 22, 2026 22:26
Copilot AI review requested due to automatic review settings July 22, 2026 23:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

tools/generateLocalizationHeader.ps1:53

  • FormatArgument(const std::basic_string<...>&) currently constructs a temporary std::string and then calls MultiByteToWide(const std::string&), which immediately calls c_str() and ignores the explicit size. This adds an unnecessary allocation/copy for non-std::string basic_string types without improving correctness.
    src/windows/wslc/services/ContainerModel.cpp:29
  • std::all_of(..., ::isdigit) is undefined behavior for chars with the high bit set because ::isdigit expects an unsigned char value (or EOF). This can mis-validate non-ASCII input depending on signedness/locale.
    static auto parsePort = [](const std::string& value, const std::wstring& errorMessage) -> uint16_t {
        try
        {
            // Ensure the value is not empty and contains only digits before parsing
            if (value.empty() || !std::all_of(value.begin(), value.end(), ::isdigit))
            {

Comment thread test/windows/SimpleTests.cpp Outdated
Comment thread test/windows/DrvFsTests.cpp Outdated
Copilot AI review requested due to automatic review settings July 23, 2026 05:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.

Comment thread tools/generateLocalizationHeader.ps1
Copilot AI review requested due to automatic review settings July 23, 2026 17:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.

Comment thread tools/generateLocalizationHeader.ps1

@dkbennett dkbennett left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its unfortunate that we have to remove this specifier; I was relying on it in the Reporter class to keep the string use simple and naturally handle wide and narrow strings. I might update the signature of the Reporter in a followup to regain this functionality but this looks fine for me. Just a minor nit about the namespace clutter where MultiByteToWide was added.

Would be nice if we could align around wide strings wherever possible and maybe we wouldn't need so many MultiByteToWide calls in the client code.

Comment thread test/windows/wslc/e2e/WSLCE2EHelpers.cpp Outdated
Ben Hillis added 4 commits July 23, 2026 14:33
Copilot-Session: a404d085-142f-448f-b7fa-7ccc6528e844
Copilot-Session: a404d085-142f-448f-b7fa-7ccc6528e844
Copilot-Session: a404d085-142f-448f-b7fa-7ccc6528e844
Copilot-Session: a404d085-142f-448f-b7fa-7ccc6528e844
Copilot AI review requested due to automatic review settings July 23, 2026 21:34
@benhillis
benhillis force-pushed the user/benhill/remove-narrow-wide-formatters branch from 032a2ce to 8930b16 Compare July 23, 2026 21:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

tools/generateLocalizationHeader.ps1:31

  • The localization FormatArgument() overload set doesn’t handle const char[N] arguments. Because generated helpers take const T& parameters, passing a narrow string literal (or other char array) will deduce T = const char[N] and fall back to the generic FormatArgument(const T&), which then forwards the array into std::make_wformat_args and fails to compile (no char[] -> wchar_t formatter). The PR description mentions supporting character arrays, so this appears to be an omission.

Comment thread test/windows/Common.h Outdated
Copilot-Session: a404d085-142f-448f-b7fa-7ccc6528e844
Copilot AI review requested due to automatic review settings July 23, 2026 22:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.

Copilot-Session: a404d085-142f-448f-b7fa-7ccc6528e844
Copilot AI review requested due to automatic review settings July 23, 2026 22:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tools/generateLocalizationHeader.ps1:36

  • On WIN32, FormatArgument currently handles char pointers and std::string, but not character arrays (e.g. string literals or const char[N]). Because the generated message helpers take arguments as const T&, string literals bind as const char(&)[N] and will fall through to the generic FormatArgument(const T&), reintroducing the deleted/removed cross-encoding formatting problem when building wide messages. This shows up in existing call sites like Localization::MessageConfigExpected("']'", ...) in src/shared/configfile/configfile.cpp.

Add an overload for const char (&)[N] (and optionally char (&)[N]) so arrays are converted via MultiByteToWide before make_wformat_args.

Copilot-Session: a404d085-142f-448f-b7fa-7ccc6528e844
Copilot AI review requested due to automatic review settings July 24, 2026 16:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 45 out of 45 changed files in this pull request and generated no new comments.

Reverts commit a21d5f1.

Copilot-Session: a404d085-142f-448f-b7fa-7ccc6528e844
Copilot AI review requested due to automatic review settings July 24, 2026 16:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

tools/generateLocalizationHeader.ps1:61

  • localization_detail::FormatArgument doesn't currently adapt char[N]/string literals on Windows. Because the generated Localization::MessageXxx templates take args as const T&, a string literal deduces T = const char[N], and overload resolution prefers the generic FormatArgument(const T&) (exact match) over FormatArgument(const char*) (array-to-pointer conversion). That leaves char[N] reaching std::make_wformat_args unconverted, which will fail to compile now that the narrow->wide std::formatter<char[N], wchar_t> specializations were removed.
    src/windows/wslc/services/ContainerModel.cpp:133
  • These THROW_HR_WITH_USER_ERROR calls use hard-coded user-facing strings. For consistency with other validation errors in this codepath (which use wsl::shared::Localization::...), consider adding localized messages for these port-range validation failures and using them here.
    if (m_containerPort.Count() == 0)
    {
        THROW_HR_WITH_USER_ERROR(E_INVALIDARG, L"Container port must specify at least one port.");
    }

Comment on lines 86 to 88
THROW_HR_WITH_USER_ERROR(
E_INVALIDARG, "Invalid protocol specified in port mapping. Only 'tcp' and 'udp' are supported.");
E_INVALIDARG, L"Invalid protocol specified in port mapping. Only 'tcp' and 'udp' are supported.");
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants